[Fix] `Android/ColorProps`: ColorProps with value null should be defaultColor instead of transparent by hank121314 · Pull Request #29830 · react/react-native

hank121314 · 2020-09-02T08:42:45Z

Summary

This pr:

Because most of @ReactProps(name = ViewProps.COLOR) accept @ Nullable Integer.
For example:
https://github.com/facebook/react-native/blob/abb6433f506851430dffb66f0dd34c1e70a223fe/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java#L472-L479

After update to react-native 0.63.2 to make PlatformColor work, there is a new ColorPropSetter.
https://github.com/facebook/react-native/blob/abb6433f506851430dffb66f0dd34c1e70a223fe/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagersPropertyCache.java#L194-L215

But ColorPropSetter won't return an nullable value with getValueOrDefault, it will always return it's defaultValue which is 0.
And 0 is equal to TRANSPARENT, will cause disappear.

Changelog

[Android] [Fixed] - ColorProps with value null should be defaultColor instead of transparent

Test Plan

Please initiated a new project and replaced the app with the following code:

import * as React from 'react';
import {Text, View, TouchableOpacity, PlatformColor} from 'react-native';

export default function App() {
  const [active, setActive] = React.useState(false);

  return (
    <View>
      <Text style={active ? {color: 'green'} : null}>Example</Text>
      <Text
        style={
          active ? {color: PlatformColor('@android:color/holo_purple')} : null
        }>
        Example2
      </Text>
      <TouchableOpacity onPress={() => setActive(!active)}>
        <Text>Toggle Active</Text>
      </TouchableOpacity>
    </View>
  );
}

Thanks you so much for your code review!

…alueOrDefault

…tType to mixed

analysis-bot · 2020-09-02T09:04:58Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	9,239,782	-11
android	hermes	armeabi-v7a	8,749,720	-5
android	hermes	x86	9,702,093	-6
android	hermes	x86_64	9,667,310	-16
android	jsc	arm64-v8a	10,886,756	-47
android	jsc	armeabi-v7a	9,787,976	-54
android	jsc	x86	10,944,476	-53
android	jsc	x86_64	11,551,223	-47

Base commit: 4246c75

analysis-bot · 2020-09-02T09:05:14Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 4246c75

safaiyeh · 2020-09-04T06:35:18Z

Nice work! @hank121314 I built RNTester to test this out:

Working as expected, tagging PR submitted on the issue.

safaiyeh · 2020-09-04T06:35:43Z

cc @JoshuaGross

hank121314 · 2020-09-05T01:24:25Z

Nice work! @hank121314 I built RNTester to test this out:

Working as expected, tagging PR submitted on the issue.

Thank you so much for help and testing! 😄

Danite · 2020-10-20T10:45:11Z

I think when the color is undefined it should also fallback to the defaultColor.

Take this case:
<Text style={[someTextStyleWithoutColor, isActive && activeTextStyle]}> Hello there! </Text>
it will make the text transparent once is not active.

hank121314 · 2020-10-21T01:09:47Z

I think when the color is undefined it should also fallback to the defaultColor.

Take this case:
<Text style={[someTextStyleWithoutColor, isActive && activeTextStyle]}> Hello there! </Text>
it will make the text transparent once is not active.

Yes, this pr is also included undefined in javascript.

analysis-bot

Code analysis results:

google-java-format found some issues. See https://github.com/google/google-java-format

analysis-bot · 2020-12-09T08:46:16Z

+    @Override
+    protected @Nullable Object getValueOrDefault(Object value, Context context) {
+      if (value != null) {
+          return ColorPropConverter.getColor(value, context);


google-java-format suggested changes:

@@ -337 +337 @@ - return ColorPropConverter.getColor(value, context); + return ColorPropConverter.getColor(value, context);

hank121314

apply google-java-format suggested changes.

Babar-Memon · 2021-02-04T08:15:32Z

i have a same problem on image component anyone can help me out to resolve this issue

hank121314 · 2021-02-04T08:29:24Z

i have a same problem on image component anyone can help me out to resolve this issue

Hi @Babar-Memon , you can try to patch this pr with Building from source.
Or set color props to any colors except null.

fabOnReact

thanks a lot @hank121314. Your pr also fixes #29378 (not linked in your pr summary), I was searching for the solution as explained in #29412 (comment) and then found your PR from another related issue.

I checkout out your branch and as in the below test case, verified that it fixes also #29378

CLICK TO OPEN TESTS RESULTS

BEFORE	AFTER

Thanks a lot. 🙏 ☮️

hank121314 · 2021-02-05T02:51:09Z

Hi @fabriziobertoglio1987 .
Thanks for your investigation and review!
Hope this pr can be merged soon. 😄

StanislavMayorov · 2021-04-07T12:17:06Z

@fabriziobertoglio1987 @hank121314 Hi. Could you merge it please?

taylorkline · 2021-06-22T21:11:10Z

This did not fix ActivityIndicator from being transparent for me. #30057 worked for me.

taylorkline · 2021-06-23T20:39:31Z

Actually, in general, this fix is not working for me on 0.64.2.

hank121314 · 2021-06-26T07:50:38Z

Hi @taylorkline, did you try to patch this pr with Building from source?
If it still does not work, could you give me some reproducible code?
Thanks!

janicduplessis

LGTM, sorry this is taking forever to merge, will ping some people at fb.

taylorkline · 2021-08-09T20:26:01Z

@hank121314 No, I did not - my apologies, likely user error on my part then.

facebook-github-bot · 2021-08-10T00:27:16Z

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-08-18T01:01:37Z

@lunaleaps merged this pull request in 842bcb9.

…or instead of transparent (#29830) Summary: This pr: - Fixes: #30183 - Fixes: #30056 - Fixes: #29950 - Fixes: #29717 - Fixes: #29495 - Fixes: #29412 - Fixes: #29378 Because most of ReactProps(name = ViewProps.COLOR) accept @ Nullable Integer. For example: https://github.com/facebook/react-native/blob/abb6433f506851430dffb66f0dd34c1e70a223fe/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java#L472-L479 After update to react-native 0.63.2 to make PlatformColor work, there is a new ColorPropSetter. https://github.com/facebook/react-native/blob/abb6433f506851430dffb66f0dd34c1e70a223fe/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagersPropertyCache.java#L194-L215 But ColorPropSetter won't return an nullable value with getValueOrDefault, it will always return it's defaultValue which is 0. And 0 is equal to TRANSPARENT, will cause <Text /> disappear. ## Changelog [Android] [Fixed] - ColorProps with value null should be defaultColor instead of transparent Pull Request resolved: #29830 Test Plan: Please initiated a new project and replaced the app with the following code: ``` import * as React from 'react'; import {Text, View, TouchableOpacity, PlatformColor} from 'react-native'; export default function App() { const [active, setActive] = React.useState(false); return ( <View> <Text style={active ? {color: 'green'} : null}>Example</Text> <Text style={ active ? {color: PlatformColor('android:color/holo_purple')} : null }> Example2 </Text> <TouchableOpacity onPress={() => setActive(!active)}> <Text>Toggle Active</Text> </TouchableOpacity> </View> ); } ``` Thanks you so much for your code review! Reviewed By: JoshuaGross Differential Revision: D30209262 Pulled By: lunaleaps fbshipit-source-id: bc223f84a92f742266cb7b40eb26722551940d76

hank121314 added 3 commits September 2, 2020 10:15

[Fix] style/color: add BoxedColorPropSetter to handle nullable getV…

ee7d46d

…alueOrDefault

Merge remote-tracking branch 'upstream/master' into fix/DynamicColorText

7867fe2

[Fix] ViewManagersPropertyCache: change BoxedColorPropSetter defaul…

664c2a9

…tType to mixed

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 2, 2020

react-native-bot added Bug Platform: Android Android applications. labels Sep 2, 2020

safaiyeh approved these changes Sep 4, 2020

View reviewed changes

hank121314 mentioned this pull request Sep 30, 2020

Default color for ActivityIndicator on Android #30057

Closed

yskoht added a commit to newn-team/react-native that referenced this pull request Oct 6, 2020

patch react#29830

eaab64a

hank121314 changed the title ~~[Fix] Android/Text: Conditionally change <Text /> color cause text invisible~~ [Fix] Android/ColorProps: ColorProps with null should be defaultColor instead of transparent Oct 13, 2020

hank121314 changed the title ~~[Fix] Android/ColorProps: ColorProps with null should be defaultColor instead of transparent~~ [Fix] Android/ColorProps: ColorProps with value null should be defaultColor instead of transparent Oct 13, 2020

hank121314 mentioned this pull request Oct 16, 2020

TintColor style isn't correctly applied when changed on Android on an Image #30183

Closed

cromatikap mentioned this pull request Oct 30, 2020

RN 0.63.1: ActivityIndicator disapearing #29378

Closed

hank121314 added 3 commits November 13, 2020 11:26

Merge branch 'master' into fix/DynamicColorText

26c1de6

Merge branch 'master' into fix/DynamicColorText

87f723e

Merge branch 'master' into fix/DynamicColorText

6db0327

analysis-bot suggested changes Dec 9, 2020

View reviewed changes

set0gut1 pushed a commit to newn-team/react-native that referenced this pull request Dec 11, 2020

patch react#29830

c1dd198

hank121314 commented Dec 14, 2020

View reviewed changes

hank121314 added 3 commits December 14, 2020 10:57

style(*): Apply google-java-format suggested changes.

cfebc44

Merge branch 'master' into fix/DynamicColorText

d506fe2

Merge branch 'master' into fix/DynamicColorText

f4f7305

hank121314 mentioned this pull request Feb 1, 2021

Android <Text> color becomes invisible when switching back to initial state #29717

Closed

Merge branch 'master' into fix/DynamicColorText

7221290

fabOnReact mentioned this pull request Feb 4, 2021

[Android] Since 0.63.1 images won't render on Android after subsequently applying & removing tintColor #29412

Closed

fabOnReact approved these changes Feb 4, 2021

View reviewed changes

fabOnReact mentioned this pull request Feb 4, 2021

ActivityIndicator works only on iOS #30056

Closed

This was referenced Feb 26, 2021

Android) Switch track is not visible when it is disabled and the parent has a dark background #30446

Open

PlatformColor does not work properly on Android #29763

Closed

hank121314 mentioned this pull request Mar 9, 2021

Fixing resetting image tintColor on Android #31114

Closed

mganandraj mentioned this pull request Mar 10, 2021

Fixing bug in Commenting icon disappering on removing tintColor property microsoft/react-native-macos#745

Merged

4 tasks

fabOnReact mentioned this pull request Apr 4, 2021

NullPointerException:tempt to invoke virtual method 'android.graphics.drawable.Drawable android.graphics.drawable.Drawable$ConstantState.newDrawable(android.content.res.Resources)' #17530

Closed

Merge branch 'master' into fix/DynamicColorText

4e45c92

JeffGuKang approved these changes Apr 14, 2021

View reviewed changes

Merge branch 'master' into fix/DynamicColorText

0a8cfc5

janicduplessis approved these changes Aug 3, 2021

View reviewed changes

janicduplessis added the Needs: React Native Team Attention label Aug 3, 2021

lunaleaps self-assigned this Aug 10, 2021

facebook-github-bot closed this in 842bcb9 Aug 18, 2021

facebook-github-bot added the Merged This PR has been merged. label Aug 18, 2021

msgharpu mentioned this pull request Oct 22, 2021

Fix for submit button disappearing bug in comments microsoft/react-native-macos#862

Merged

3 tasks

Conversation

hank121314 commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

safaiyeh commented Sep 4, 2020

Uh oh!

safaiyeh commented Sep 4, 2020

Uh oh!

hank121314 commented Sep 5, 2020

Uh oh!

Danite commented Oct 20, 2020

Uh oh!

hank121314 commented Oct 21, 2020

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

analysis-bot Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

hank121314 left a comment

Choose a reason for hiding this comment

Uh oh!

Babar-Memon commented Feb 4, 2021

Uh oh!

hank121314 commented Feb 4, 2021

Uh oh!

fabOnReact left a comment

Choose a reason for hiding this comment

Uh oh!

hank121314 commented Feb 5, 2021

Uh oh!

StanislavMayorov commented Apr 7, 2021

Uh oh!

taylorkline commented Jun 22, 2021

Uh oh!

taylorkline commented Jun 23, 2021

Uh oh!

hank121314 commented Jun 26, 2021

Uh oh!

janicduplessis left a comment

Choose a reason for hiding this comment

Uh oh!

taylorkline commented Aug 9, 2021

Uh oh!

facebook-github-bot commented Aug 10, 2021

Uh oh!

facebook-github-bot commented Aug 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

hank121314 commented Sep 2, 2020 •

edited

Loading

analysis-bot commented Sep 2, 2020 •

edited

Loading

analysis-bot commented Sep 2, 2020 •

edited

Loading